-
Notifications
You must be signed in to change notification settings - Fork 358
Update friendsofphp/php-cs-fixer requirement from ^2.19 to ^3.40 #1520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Update friendsofphp/php-cs-fixer requirement from ^2.19 to ^3.40 #1520
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1520 +/- ##
============================================
- Coverage 97.24% 97.22% -0.02%
Complexity 2834 2834
============================================
Files 175 175
Lines 8843 9018 +175
============================================
+ Hits 8599 8768 +169
- Misses 244 250 +6 ☔ View full report in Codecov by Sentry. |
|
Updated so that it uses php-cs-fixer major version 3 (no v2 any more). version 3 supports PHP 7.4 and 8, so that works fine. php-cs-fixer v3 does lots of thing like: It changed lots of stuff, which I had to push in the 2nd commit. We dropped older PHP in master branch, so dependabot correctly noticed that we can move to php-cs-fixer v3 in master. That is good, because we want to start adding more types, phpstan doc etc. for master to move forward to be a next major release "some day". |
|
@DeepDiver1975 @staabm I don't think that you need to do "too long" review of this, because is it just "what php-cs-fixer" does. But I would appreciate your feedback to say that you think it is reasonable to do. |
4c7ca99 to
9757809
Compare
9757809 to
5fbe1da
Compare
| * calendar-data. If the result of a subsequent GET to this object is not | ||
| * the exact same as this request body, you should omit the ETag. | ||
| * | ||
| * @param mixed $calendarId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like having explicit docs about mixed types. it allowes the api user to differentiate where we accidentally forgot to define a type and where we explicitly declared mixed.
maybe this is a rule we could skip?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this is a rule we could skip?
agree - explicitly writing "mixed" means that someone should have thought about it and that mixed is really allowed.
https://cs.symfony.com/doc/rules/phpdoc/no_superfluous_phpdoc_tags.html
I pushed a commit to allow mixed PHPdoc tags.
but what do we do about existing places where mixed is in the PHPdoc?
For example, I doubt that $calendarId can be anything. It should have a better PHPdoc clue than mixed
So the existing places that have mixed in PHPdoc (and were auto-removed in this PR) maybe do not actually allow the full set of mixed as inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, I doubt that
$calendarIdcan be anything. It should have a better PHPdoc clue thanmixed
I assume we have releases with such phpdocs - which means I am fine with it.
separate PRs can be submitted to improve/narrow these places if possible.
|
@staabm please have a "quick" look. Let me know if you are OK with moving the code forward like cs-fixer suggests. |
| /** | ||
| * Creates the email handler. | ||
| * | ||
| * @param string $senderEmail. The 'senderEmail' is the email that shows up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we lost a param type with this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed. I think that the "." on the end of the varibale name was what confused cs-fixer.
And I accidentally removed the whole line!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment still missing ....
|
(reviewed with the wrong account ;-)) |
|
A newer version of friendsofphp/php-cs-fixer exists, but since this PR has been edited by someone other than Dependabot I haven't updated it. You'll get a PR for the updated version as normal once this PR is merged. |
e6b625c to
5909f2e
Compare
|
Rebased and applied changes suggested by php-cs-fixer 3.58.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a quick scan and left a comment or two ....
| $counter = 0; | ||
|
|
||
| while ($row = $result->fetch(\PDO::FETCH_ASSOC)) { | ||
| while ($row = $result->fetch(PDO::FETCH_ASSOC)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the leading \ removed? Is the speed benefit on lookup no longer of interest? 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no speed benefit. This script runs in the global namespace, so there's no point in prefixing anything with a \.
| * | ||
| * @param DateTime $start | ||
| * @param DateTime $end | ||
| * @param \DateTime $start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.... and here \ is added .....
| /** | ||
| * Creates the email handler. | ||
| * | ||
| * @param string $senderEmail. The 'senderEmail' is the email that shows up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment still missing ....
just a random idea while looking at the PR. if its not too much work, it might make sense to separate the test/ changes from the rest (to ease review) |
|
how do we move on? @phil-davis @clxmstaab @staabm - THX |
|
I am fine with moving ahead/merge. there is no value in arguing about CS. @phil-davis any strong opinions? |
|
The |
Updates the requirements on [friendsofphp/php-cs-fixer](https://github.com/PHP-CS-Fixer/PHP-CS-Fixer) to permit the latest version. - [Release notes](https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/releases) - [Changelog](https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/master/CHANGELOG.md) - [Commits](PHP-CS-Fixer/PHP-CS-Fixer@v2.19.0...v3.40.0) --- updated-dependencies: - dependency-name: friendsofphp/php-cs-fixer dependency-type: direct:development ... Signed-off-by: dependabot[bot] <[email protected]>
161ceac to
5615c3f
Compare
I will look through this again to see what state it is in, and address various comments above. So I have set to Draft for now. |
Updates the requirements on friendsofphp/php-cs-fixer to permit the latest version.
Release notes
Sourced from friendsofphp/php-cs-fixer's releases.
Changelog
Sourced from friendsofphp/php-cs-fixer's changelog.
... (truncated)
Commits
27d2b32prepared the 3.40.0 release45a4530chore: officially support PHP 8.3 (#7466)d50d5b6CI: move humbug/box out of dev-tools/composer.json (#7472)9ae580fCI: automate --ignore-platform-req=PHP (#7467)9fbe27echore: update deps (#7471)0a3a684CI: add --no-update while dropping non-compatfacile-it/paraunit(#7470)0bc713dCI: bump actions/github-script to v7 (#7468)7735bbfbumped version857046dprepared the 3.39.1 releaseffc5780fix: OrderedInterfacesFixer - do not comment out interface (#7464)You can trigger a rebase of this PR by commenting
@dependabot rebase.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot mergewill merge this PR after your CI passes on it@dependabot squash and mergewill squash and merge this PR after your CI passes on it@dependabot cancel mergewill cancel a previously requested merge and block automerging@dependabot reopenwill reopen this PR if it is closed@dependabot closewill close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot show <dependency name> ignore conditionswill show all of the ignore conditions of the specified dependency@dependabot ignore this major versionwill close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor versionwill close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependencywill close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)